STDO-124: Fix NPE and test-isolation failures - #33
Conversation
PropertiesLoader.readFolder NPEd on a null listFiles() result; hardened to treat null as no files. FigUtilsTest left the process-wide Fig singleton mutated with no teardown, causing four unrelated MergedPropertiesLoaderTest failures depending on run order. Added an @after reset. Found while verifying a clean build from an empty local Maven repo as part of the STDO-124 source-handoff bet.
alextwigkit
left a comment
alextwigkit
left a comment
There was a problem hiding this comment.
🔍 Deep PR Review
This PR hardens PropertiesLoader.readFolder() against a NullPointerException when File.listFiles() returns null, and fixes order-dependent test failures by adding an @After teardown to FigUtilsTest that reloads a shared Fig singleton FigUtils.merge() mutates in place. Both fixes are independently confirmed correct and well-targeted at their root causes; the main gap is that neither fix ships with a regression test, so a future refactor could silently reintroduce either bug. One pre-existing, structurally identical null-deref risk in the same file's delete() method was also surfaced but is out of scope for this diff.
| Finding | File | Severity | Source |
|---|---|---|---|
| New null-guard branches for folder.listFiles() have no regression test anywhere in the repo | PropertiesLoader.java:93-132 |
P2 | test-coverage, logical-coverage |
| Singleton-isolation fix has no automated test proving it prevents the cross-class failure it targets | FigUtilsTest.java:17-33 |
P3 | test-coverage |
Specialists run: best-practices, security, test-coverage, ticket-fidelity, ripple-effect, logical-coverage, historical-patterns
See inline comments for details.
📋 Below threshold (1 item — your call whether to act)
| Finding | File | Detail |
|---|---|---|
| Fail-open on unreadable config directories can silently and permanently omit security-relevant configuration | PropertiesLoader.java:93 |
Real behavior change, but no demonstrated consumer stores security-relevant config in Fig, and the failure is logged, not silent [security, confidence: 40] |
📂 Pre-existing issues not introduced by this PR (1 item)
| Finding | File | Severity | Source |
|---|---|---|---|
| delete() has the same unguarded listFiles() call the PR just hardened elsewhere in this file | PropertiesLoader.java:195 |
P3 | best-practices, ripple-effect |
💪 Strengths
- Root cause correctly diagnosed and fixed, not masked — the shared
Figsingleton leak is fixed viaFig.reload(), which fully discardsFigUtils.merge()'s in-place mutation rather than reordering tests or papering over the symptom (independently confirmed:reload()rebuildsconfigsfrom scratch via the loaders). - Consistent fix across both call sites — both
listFiles()calls inreadFolder()are hardened, not just the one that happened to be observed failing. - Clear Javadoc on the test fix — the
FigUtilsTestteardown's comment explains precisely why the reload is necessary, saving future maintainers from re-diagnosing the same issue. - Concrete, falsifiable verification claim — the PR description states
mvn clean packagefrom an empty repo, twice, 54 tests/0 failures/0 errors. - Minimal, behavior-preserving diff — no unrelated refactoring bundled into this bug-fix PR.
Responses to review comments are analyzed in future reviews of this repo — your feedback shapes what gets flagged.
Review powered by deep-pr-review v2.3.4 • 7 specialists • 2 valid findings
| @@ -123,6 +127,9 @@ public boolean accept(File file) { | |||
| }; | |||
|
|
|||
| File[] nestedFolders = folder.listFiles(folderFilter); | |||
| if (nestedFolders == null) { | |||
| nestedFolders = new File[0]; | |||
| } | |||
There was a problem hiding this comment.
[P2 — high] New null-guard branches for folder.listFiles() have no regression test anywhere in the repo
Neither new null-guard branch (lines 93-97 for the file-listing call, lines 129-132 for the nested-folder-listing call) is exercised by any test. fig-core/pom.xml has no Mockito/EasyMock/PowerMock or similar mocking dependency, and every existing test in PropertiesLoaderTest.java/MergedPropertiesLoaderTest.java passes an already-confirmed real directory into readFolder, so listFiles() never returns null in the current test suite.
Tip
Suggested: add a unit test that points a PropertiesLoader at a File that is not a directory (so listFiles() returns null per the java.io.File contract) and assert readFolder/load completes without throwing. This is the cheapest way to hit the guard without adding a mocking library dependency.
💡 Copy this prompt to fix with Claude Code
In twigkit/fig on the branch for PR #33, fix this issue:
File: fig-core/src/main/java/twigkit/fig/loader/PropertiesLoader.java
Line(s): 93-132
Problem: New null-guard branches for folder.listFiles() have no regression test anywhere in the repo
### [P2 — high] New null-guard branches for folder.listFiles() have no regression test anywhere in the repo
Neither new null-guard branch (lines 93-97 for the file-listing call, lines 129-132 for the nested-folder-listing call) is exercised by any test. fig-core/pom.xml has no Mockito/EasyMock/PowerMock or similar mocking dependency, and every existing test in PropertiesLoaderTest.java/MergedPropertiesLoaderTest.java passes an already-confirmed real directory into readFolder, so listFiles() never returns null in the current test suite.
> [!TIP]
> Suggested: add a unit test that points a PropertiesLoader at a File that is not a directory (so listFiles() returns null per the java.io.File contract) and assert readFolder/load completes without throwing. This is the cheapest way to hit the guard without adding a mocking library dependency.
After fixing, respond to the review comment on PR #33 in twigkit/fig
confirming the fix. Finding: "New null-guard branches for folder.listFiles() have no regression test anywhere in the repo" in fig-core/src/main/java/twigkit/fig/loader/PropertiesLoader.java.
| /** | ||
| * {@link Fig#getInstance(twigkit.fig.loader.Loader...)} returns a process-wide singleton | ||
| * keyed on the loader(s) used. {@link FigUtils#merge(Fig, Fig)} mutates its first | ||
| * argument in place, so merging into the singleton for "confs" here would otherwise | ||
| * permanently leave that shared instance with merged-in data for the rest of the test | ||
| * run, corrupting unrelated tests (e.g. in {@code MergedPropertiesLoaderTest}) that | ||
| * expect to see the pristine "confs" configuration. Reloading after each test restores | ||
| * the singleton to its original, unmerged state. | ||
| */ | ||
| private Fig primary; | ||
|
|
||
| @After | ||
| public void restoreSharedPrimaryFig() { | ||
| if (primary != null) { | ||
| primary.reload(); | ||
| } | ||
| } |
There was a problem hiding this comment.
[P3 — medium] Singleton-isolation fix has no automated test proving it prevents the cross-class failure it targets
By inspection this teardown is correct: Fig.getInstance keys its singleton map on loader path, so FigUtilsTest and MergedPropertiesLoaderTest share the same "confs" Fig instance, and Fig.reload() rebuilds configs from scratch, discarding FigUtils.merge()'s in-place mutation. However, no test in the repo mechanically forces FigUtilsTest to run immediately before MergedPropertiesLoaderTest and asserts the latter still observes pristine "confs" data — the regression this PR targets is guarded only by the PR description's manual "isolated before/after" verification, not by an automated check.
Tip
Suggested: add a combined-suite test (or @FixMethodOrder) that runs a FigUtilsTest-style merge against the shared "confs" singleton, then re-asserts the pristine values a MergedPropertiesLoaderTest case depends on — converting the current one-time manual verification into a standing regression check.
💡 Copy this prompt to fix with Claude Code
In twigkit/fig on the branch for PR #33, fix this issue:
File: fig-core/src/test/java/twigkit.fig/util/FigUtilsTest.java
Line(s): 17-33
Problem: Singleton-isolation fix has no automated test proving it prevents the cross-class failure it targets
### [P3 — medium] Singleton-isolation fix has no automated test proving it prevents the cross-class failure it targets
By inspection this teardown is correct: Fig.getInstance keys its singleton map on loader path, so FigUtilsTest and MergedPropertiesLoaderTest share the same "confs" Fig instance, and Fig.reload() rebuilds configs from scratch, discarding FigUtils.merge()'s in-place mutation. However, no test in the repo mechanically forces FigUtilsTest to run immediately before MergedPropertiesLoaderTest and asserts the latter still observes pristine "confs" data — the regression this PR targets is guarded only by the PR description's manual "isolated before/after" verification, not by an automated check.
> [!TIP]
> Suggested: add a combined-suite test (or @FixMethodOrder) that runs a FigUtilsTest-style merge against the shared "confs" singleton, then re-asserts the pristine values a MergedPropertiesLoaderTest case depends on — converting the current one-time manual verification into a standing regression check.
After fixing, respond to the review comment on PR #33 in twigkit/fig
confirming the fix. Finding: "Singleton-isolation fix has no automated test proving it prevents the cross-class failure it targets" in fig-core/src/test/java/twigkit.fig/util/FigUtilsTest.java.
1 participant
Found while verifying a clean build from an empty local Maven repository as part of the STDO-124 Legacy App Studio source-handoff bet: fig is one of six sibling repos this bet is preparing versioned releases of.
What was fixed
PropertiesLoader.readFolderNPEd whenFile.listFiles()returnednull(happens when the directory can't be listed, e.g. a permissions restriction). Hardened to treat anullresult as no files rather than crashing.MergedPropertiesLoaderTestfailures, order-dependent:FigUtilsTestmutates the process-wideFig.getInstance()singleton and never restores it, so a class running afterward in the same JVM inherits a polluted singleton. Added an@Afterteardown that resets it.Both confirmed by isolating the failing tests before and after the fix, not by inference — running each test class alone reproduced/resolved the failure independently of run order.
Verified
mvn clean packagefrom an empty local repository, twice after the fix: exit 0 both times, 54 tests run, 0 failures, 0 errors.Part of the STDO-124 Legacy App Studio perpetual handoff bet; see
PLAN.mdinlucidworks/tbe-pitches(STDO-124-bet) for the full plan.